Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify useResizeObserver #64820

Merged
merged 2 commits into from
Aug 30, 2024
Merged

Simplify useResizeObserver #64820

merged 2 commits into from
Aug 30, 2024

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Aug 27, 2024

I've been investigating a bug in WordPress.com where Gutenberg pattern previews trigger a ResizeObserver infinite loop:

ResizeObserver loop completed with undelivered notifications.

I started by looking at how useResizeObserver works, and discovered that it's unnecessarily complex. I decided to remove the unneeded features, which ended up with an almost complete rewrite.

  • the only place that uses the useResizeObserver hook is the useResizeAware hook that creates a helper element with ResizeObserver attached to it. This usage doesn't use any opts, so they can be removed. Only the contentBoxSize entry is ever used, so we don't need to support any other. We also don't use the custom round function.
  • the useResolvedElement and its special refOrElement option is not needed. I'm replacing that with simple logic that mounts the ResizeElement component, stores a local ref to the inner div, and then creates the observer in a layout effect.
  • the didUnmountRef can also be removed, because React 18 no longer complains when setState is called after unmount.

What do you think?

@jsnajdr jsnajdr added [Type] Code Quality Issues or PRs that relate to code quality [Package] Compose /packages/compose labels Aug 27, 2024
@jsnajdr jsnajdr requested review from youknowriad and a team August 27, 2024 08:35
@jsnajdr jsnajdr self-assigned this Aug 27, 2024
Copy link

github-actions bot commented Aug 27, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ciampo
Copy link
Contributor

ciampo commented Aug 27, 2024

cc @DaniGuardiola in particular who's also been looking at useResizeObserver code recently (see #62946 (comment))

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 27, 2024

The useResizeObserver that @DaniGuardiola proposed in #62946 is used like:

useResizeObserver( element, ( element ) => { ... } );

It starts observing the element resizes, and will invoke the callback whenever the ResizeObserver reports a resize. It ignores the entries passed to the ResizeObserver callback. Instead of using the provided sizes, you are supposed to measure the element yourself.

There's also a fireOnObserve option. When enabled, the callback will be called also on mount. That can be helpful for keeping track of the element size even when it didn't resize yet:

const [ size, setSize ] = useState();
useResizeObserver( element, ( element ) => setSite( measure( element ) ) );

Here the size state contains valid size info right after mount (in the on-mount effect).

Curiously, the useResizeObserver in compose will report { width: null, height: null } size until a resize really happens.

If we wanted, I think we could have both hooks: the basic one that fires a callback on resize, and then another one that builds on top of the first one, adding the overlay element and measuring that element specifically.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a look, and it's impressive how much simpler the code is 👏

discovered that it's unnecessarily complex

I'd be curious to know if there were specific use cases when the current implementation was added to the repo (#40509), which caused that implementation to be that complex — maybe @youknowriad has more context around this.

The code changes look, but I assume this would need some good manual testing (js-dom based tests won't pick up any changes, I guess, and I'm not sure how well e2e tests cover this hook).

We could also tentatively merge these changes and assume that there is enough time before the next WordPress release that any blocking bugs that we don't catch while reviewing would still be flagged soon.

} else if ( entry.contentBoxSize[ 0 ] ) {
const contentBoxSize = entry.contentBoxSize[ 0 ];
entrySize = [ contentBoxSize.inlineSize, contentBoxSize.blockSize ];
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make the logic more explicit about checking that entry.contentBoxSize is an array or an object?

@ciampo
Copy link
Contributor

ciampo commented Aug 28, 2024

The useResizeObserver that @DaniGuardiola proposed in #62946 is used like [...] Instead of using the provided sizes, you are supposed to measure the element yourself.

@DaniGuardiola , I don't remember if there was a specific reason why you chose to measure the element yourself, instead of using the measurements from the RO. Using the measumerements from the RO could result in better runtime performance.

There's also a fireOnObserve option. When enabled, the callback will be called also on mount. That can be helpful for keeping track of the element size even when it didn't resize yet:

Is there ever a scenario in which we wouldn't want the hook to fire on mount? On paper that sounds like a feature that we'd benefit from, regardless.

If we wanted, I think we could have both hooks: the basic one that fires a callback on resize, and then another one that builds on top of the first one, adding the overlay element and measuring that element specifically.

That is something that we should explore, although not urgent IMO.

@DaniGuardiola
Copy link
Contributor

I'll elaborate tomorrow as today I am AFK due to being sick, but in short:

  • I measured independently because the specific use-case required something not provided by RO entries.
  • My utility, iirc, forwards the RO entries, so that's completely doable.
  • I investigated and it turns out RO is supposed to also trigger initially, so as long as that is true, my hook can be simplified.
  • My hook is reactive to "element", which was a requirement (a ref is not stateful and therefore would fail said requirement). I believe these utils could be based upon my hook so we could get both things. Create the sizing element internally, obtain it's hook, and let my hook abstract the RO as needed. Would only need to accept RO options if it doesn't already, I don't remember.

I'll take a closer look tomorrow :)

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 29, 2024

Is there ever a scenario in which we wouldn't want the hook to fire on mount?

Turns out that the answer is no, because ResizeObserver always fires an observation right after the .observe function is called. You always get an "initial size" without any extra work.

I measured independently because the specific use-case required something not provided by RO entries.

el.getBoundingClientRect should be equivalent to entry.borderBoxSize, but there is one important exception: ResizeObserver always ignores CSS transforms. If your div has transform: scale(2), then ResizeObserver will report the untransformed size, while getBoundingClientRect() will return a rectangle that's twice as big.

But ResizeObserver won't inform you when the transform changes, so the transformed size from .getBoundingClientRect is not 100% reliable.

The spec summarizes it as:

Screenshot 2024-08-29 at 13 01 13

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 30, 2024

I added one more improvement: the ResizeObserver callback provides an array of entries, in case multiple resizes happened before the callback had the chance to run. But so far we've been looking only at entries[ 0 ]. If there were further entries, newer and more up-to-date, we were ignoring them. The measured size can become stale. I'm replacing that with a loop that processes all entries.

All examples in the spec and on MDN do a loop like this. Maybe we could look at just the last entry, but I decided to follow the examples and process each entry fully.

I think this PR is now ready to be merged. If you don't have any major objections, let's approve and ship it.

@jsnajdr jsnajdr force-pushed the simplify/use-resize-observer branch from 91eac50 to 1e305a9 Compare August 30, 2024 07:01
@ciampo
Copy link
Contributor

ciampo commented Aug 30, 2024

el.getBoundingClientRect should be equivalent to entry.borderBoxSize, but there is one important exception: ResizeObserver always ignores CSS transforms. If your div has transform: scale(2), then ResizeObserver will report the untransformed size, while getBoundingClientRect() will return a rectangle that's twice as big.

But ResizeObserver won't inform you when the transform changes, so the transformed size from .getBoundingClientRect is not 100% reliable.

Yup, and that is (I believe) one of the main reasons why @DaniGuardiola doesn't use the RO's measurements in the implementation linked above — because we need those measurements to work properly even when transforms are applied.

I think this PR is now ready to be merged.

Code is LGTM, but let's wait for @DaniGuardiola 's last round of feedback, since he mentioned

I'll take a closer look tomorrow :)

@DaniGuardiola
Copy link
Contributor

Sorry for not commenting yesterday, but I wanted to finish up a PR I'm about to put up where I've simplified my hook, and I believe it should be possible to conciliate that version with this effort. I'll share more in a bit, once that's ready :)

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the cleanup 💯

This appears to work well in my tests 👍

I'm curious if we have any concerns about removing those APIs that useResizeObserver was supporting but we weren't really using in Gutenberg.

Also, let's wait for @DaniGuardiola's update / review.

@DaniGuardiola @jsnajdr you folks might want to work on this together in a single branch 😉 Why approach this separately?

// behaviour of returning objects instead of arrays for `borderBoxSize` and `contentBoxSize`.
// @ts-ignore
entry[ boxProp ][ sizeType ];
const [ width, height ] = entrySize.map( ( d ) => Math.round( d ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any use of the custom rounding function, but I assume we'll be fine with always using the default one?

@DaniGuardiola
Copy link
Contributor

@tyxla to clarify, my upcoming PR does not touch these files, and rather it updates my util along with the Tabs indicator animation (and the upcoming ToggleGroupControl animation which is done and depends on these changes too). The goal is to merge that PR to then compare with what's going on in this PR and see if we can finally unify everything under here.

I can't write more details for now (heading to lunch) but the PR is done. Will undraft once I can write more details in its description, but you can look at the refactor I did of the utils for now: #64926

@DaniGuardiola
Copy link
Contributor

Update: PR is ready for review (#64926).

Copy link
Contributor

@DaniGuardiola DaniGuardiola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and my needs can be covered in a follow-up PR. Just left a comment about naming and with details about what I want to contribute next.

/>
);
return [ resizeListener, sizes ];
export default function useResizeObserver(): [ ReactElement, ObservedSize ] {
Copy link
Contributor

@DaniGuardiola DaniGuardiola Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not properly called in an ideal world, it should rather be named useResizeAware since it's not a direct wrapper over ResizeObserver, but rather a utility that returns an element with specific constraints that is measured, and the resulting measurements.

The unfortunate thing is that the path is named after "useResizeObserver" even though the actual exported utility is "useResizeAware", thanks to the default export which makes things nameless. Then, it is exported as "useResizeObserver" under a named export in the outer barrel file (index.js).

Regarding this utility, a lot of what you could do with a proper, thin wrapper of ResizeObserver can not be achieved with this, like what I do in the animation of Tabs and ToggleGroupControl that I worked in (see latest version in #64926).

However, it should be possible to have everything we need by moving my thin wrapper here, and then using it as a base for this utility. That way, we unlock all use cases instead of only this specific one.

Anyway, as stated above, it seems like the name is irremediably taken, unfortunately. Maybe the lower-level hook (what would be my hook) could be called useObserveElementSize(element, onUpdate, resizeObserverOptions).

Update: I added my hook in this PR - #64943

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the comments above, it looks like there is agreement on merging these changes, and iterating once Dani's PR is merged

@DaniGuardiola
Copy link
Contributor

I created a PR that targets this one (so that it can be merged after this one) that clarifies what I intend to add: #64943 (useEvent and useObserveElementSize).

Then, a final follow-up would be to remove the util from the components package to de-duplicate (see #64926 for the last version of the utility I mean, which is exactly the same as useObserveElementSize but named useResizeObserver).

@jsnajdr jsnajdr merged commit 8a6903f into trunk Aug 30, 2024
67 of 68 checks passed
@jsnajdr jsnajdr deleted the simplify/use-resize-observer branch August 30, 2024 17:23
@github-actions github-actions bot added this to the Gutenberg 19.2 milestone Aug 30, 2024
bph pushed a commit to bph/gutenberg that referenced this pull request Aug 31, 2024
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Compose /packages/compose [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants